feat: install.sh - #59
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
I'd like to check the go.mod path and fix the error surfacing that could have a wrong install leaving the shell half configured
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeWell-structured install script modelled on established installers (uv, rustup). Two issues from the existing review threads need addressing before merge: All checks from the latest CI run passed; the cancelled checks are from a superseded run.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Automate: a CI job that runs the installer against a goreleaser snapshot (already added in the release workflow; consider also running it on PR CI with a mock server). Product take: This is the primary binary distribution channel for the Go CLI rewrite — essential for adoption. The script itself is solid and follows established conventions. Once the two Majors are addressed, it's ready to ship. 🧭 Assumptions & unverified claims
A script that diligently verifies checksums but then |
The script pins the version it was released alongside, so the common path makes no API call and cannot be rate limited: release-please rewrites DEFAULT_VERSION through the `extra-files` entry, whose Generic updater replaces the first semver token on any line carrying `x-release-please-version`. Only the token changes, so the leading v survives. Everything lives in functions with `main "$@"` last, so a truncated download cannot execute half a script. The strict curl flags apply to https URLs only — FLAGSMITH_CLI_BASE_URL exists to point the script at a local server for testing, and that server is plain http. PATH handling follows uv: an env script the startup files source, edited once each, skipped when the install dir is already on PATH. The env script lives under our own directory because $HOME/.local/bin/env belongs to cargo-dist and uv writes it. beep boop
Each `run:` step gets a fresh shell, and an Actions shell is neither a login nor an interactive one, so nothing it starts reads the startup files the installer edited. Appending the directory to the file $GITHUB_PATH points at is the mechanism the runner does honour: it prepends those entries for every later step. So `curl ... | sh` in one step and `flagsmith` in the next now works on any runner, rather than only on images that already ship ~/.local/bin on PATH. Taken from uv's Add-Ci-Path. The release smoke job drops its explicit sourcing of the env script and relies on this instead. beep boop
POSIX ignores -e for every command of an AND-OR list but the last, and that suppression covers the entire body of a function called there. So `main "$@" || exit 1` disabled set -e for the whole script: tar, mkdir, chmod and mv could all fail unnoticed. Reinstalling over an existing binary with a corrupt archive printed the success message and exited 0. The idiom came from rustup and cargo-dist, which set -u only and wrap every command in `ensure`, so the trailing `|| exit 1` costs them nothing. It is not transferable to a script that relies on -e. beep boop
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
Blindly stamping based on previous approval which was dismissed by a rebase 🙈
3 participants
Closes #44.
curl -fsSL https://raw.githubusercontent.com/Flagsmith/flagsmith-cli/main/install.sh | shThe script installs to
$HOME/.local/bin, verifies the sha256 fromchecksums.txt, and adds the install directory toPATHif it's not there.The script pins the version it shipped with. The pin is bumped by release-please.
Verified locally
Against a goreleaser snapshot served over http, into a sandboxed
HOME:.profilesources it, and sourcing it putsflagsmithonPATH--no-modify-pathwrites neither the rc line nor the env scriptchecksum mismatchexpected exactly one checksumvall behaveImportant
DEFAULT_VERSIONisv2.0.0-beta.1, which is what ci: configure release-please for the 2.0.0 beta line #54'sRelease-As:footer forces.gh attestation verify --owner Flagsmith <file>— but the script doesn't check it, since users won't haveghinstalled.curl | shis unaffected (curl sets no quarantine xattr); a browser download on macOS will be Gatekeeper-blocked.install.ps1for Windows is deliberately out of scope — the script tells Windows users to take the.zip.add_ci_pathappends the install dir to$GITHUB_PATH, socurl … | shin one Actions step leaves the CLI on PATH for later steps — an Actions shell is neither login nor interactive, so it never reads the startup files the installer edits. Taken from uv. The release smoke job relies on it rather than sourcing the env script.